Implements a cache for train collision checks for better performance #9513

stacode123 · 2025-11-08T14:19:47Z

Implements a cache for train collision checks for better performance
Before: https://spark.lucko.me/cgjtsUl25f
After: https://spark.lucko.me/p7ZcDxAUFA
Criticism welcome.
Made with the help of Copilot

World used for testing :
world.zip

…e networks

sleepy-evelyn · 2025-11-08T18:07:23Z

If this works properly this is brilliant @stacode123 and definately needed. We've turned train collisions off entirely to help with lag but thats definately not a fix most servers want to implement.

Using Copilot is a little worrying though because you need to know exactly whats going on for this. Make sure you've tested it thoroughly and used the debugger to step through the code to see whats going on and if it aligns with what you expect.

Jozufozu

Very excited about this and the spark profile is extremely promising. A 5mspt difference is huge

How many trains did you use in your test world? It would be helpful to upload a screenshot of the chaos 😆

Also after the requested changes and before this merges I'd like to request a longer profile to compare. I'm a little nervous the 16 second profiles may not have enough signal to noise

src/main/java/com/simibubi/create/content/trains/entity/Train.java

Jozufozu · 2025-11-08T21:21:04Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

+			if (length > 0.0001) {
+				direction[index] = diff.normalize();
+			} else {
+				direction[index] = Vec3.ZERO;


How will this happen?

I see you've removed this but please justify it

stacode123 · 2025-11-08T21:36:24Z

test world has 180 trains more than that would have caused my computer to have a seizure

Will address the comments

stacode123 · 2025-11-09T11:27:43Z

Performance impact on a server with 475 trains:

Before:

After:

sleepy-evelyn · 2025-11-09T16:07:33Z

To be honest it's rare u see such a stark contrast between before and after. A similar number of players are connected to the server as well so the difference likely isn't down to that. :)

…llisionCache calls.

stacode123 · 2025-11-09T19:04:32Z

@Jozufozu Please review the changes :D

Jozufozu · 2025-11-09T19:46:44Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java


-		Vec3 start = (speed < 0 ? trailingPoint : leadingPoint).getPosition(graph);
-		Vec3 end = (speed < 0 ? leadingPoint : trailingPoint).getPosition(graph);
+		Vec3 start = collisionCache.start[0];


Where did the speed < 0 checks go? If the train is moving backwards we need to check for collisions with the trailing end as if it were the head

Oh also do we know for sure that the collision cache is non-null here?

That is handled when collision cache is created.

Earlier in the train tick we updated the collision cache but i can add a null check just to be sure

I do not see anything in this function that would indicate the order of the cache is reversed based on speed https://github.com/Creators-of-Create/Create/pull/9513/files#diff-671a8e994774f6be471b30fd8459587f56fd246119f83bb82b47b47e0056608cR646

Earlier in the train tick we potentially invalidate the cache, leaving it null

src/main/java/com/simibubi/create/content/trains/entity/Train.java

Jozufozu · 2025-11-09T19:50:42Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

+			if (length > 0.0001) {
+				direction[index] = diff.normalize();
+			} else {
+				direction[index] = Vec3.ZERO;


I see you've removed this but please justify it

src/main/java/com/simibubi/create/content/trains/entity/Train.java

Jozufozu · 2025-11-09T19:54:26Z

src/main/java/com/simibubi/create/content/trains/GlobalRailwayManager.java

+		for (Train train : movingTrains){
+			train.updateCollisionCache();
+			train.earlyTick(level);}


formatting please

for (Train train : movingTrains) { train.updateCollisionCache(); train.earlyTick(level); }

do we not need the collision cache for waiting trains? do we not expect moving trains to collide with trains that are stationary?

Wont stationary trains have already built up cache ? And trying to calculate it again would just yield the same result?

What about on initial level load? Can we really assume that a train that just became stationary this tick will have valid positions in the cache from the previous tick where it was moving?

Still, that's a good point. If you're feeling ambition you could probably check some easy to compute value at the start of updateCollisionCache to see if the train has moved from the previous tick and decide whether to update the collision cache. If you do though, I'd like to see proof that it works and waiting trains still get collided with

1 Initial level load will have the cache empty, causing it to update at the start of the collision checks.
2. I think so? we have been running the build on our server with no issues for now.
3. That would the best direction

Where will it update? On this line you specifically skip trains that do not have a collision cache: https://github.com/Creators-of-Create/Create/pull/9513/files#diff-671a8e994774f6be471b30fd8459587f56fd246119f83bb82b47b47e0056608cR732

I believe it doesn't crash or anything, but I'm not convinced that there aren't false negatives in the collision checks under the current iteration of this PR

I agree, do it

Jozufozu · 2025-11-09T19:54:57Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

 		if (derailed || graph == null) {
 			if (collisionCache != null)
-				collisionCache.invalidate();
+				collisionCache = null;


probably don't need the collisionCache != null check anymore. just assigning is fine

what do you mean by that

src/main/java/com/simibubi/create/content/trains/entity/Train.java

Jozufozu · 2025-11-09T19:55:36Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

+	private void updateCollisionCache() {
+		if (derailed || graph == null) {
+			if (collisionCache != null)
+				collisionCache.invalidate();


Add a comment in the code please

Jozufozu · 2025-11-22T21:35:21Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

+		//
 		if (derailed || graph == null) {
 			if (collisionCache != null)
-				collisionCache = null;


Shouldn't we still be setting this to null before returning?
This is why I asked you to explain why it was being set to null here, so that future maintainers can understand the intention.

My understanding of your intention is that a null collision cache means there is nothing valid to collide with. Here you explicitly leave a cache with potentially stale positions in a valid state

Jozufozu · 2025-11-22T21:35:32Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java



 	public void updateCollisionCache() {
+		//


Did you mean to write something here?

Jozufozu · 2025-11-22T21:36:01Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

 			collisionCache = new CollisionCache(maxExpectedSegments);
 		}

+		//For adding segments between carriages


// For adding segments between carriages
Leave a space after the // please

Jozufozu · 2025-11-22T21:37:49Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java

 				continue;
 			if (train.graph != null && train.graph != graph)
 				continue;
+			if (train.collisionCache == null) {continue;}


Formatting

if (train.collisionCache == null) continue;

or

if (train.collisionCache == null) { continue; }

but be consistent

Jozufozu · 2025-11-22T21:48:48Z

src/main/java/com/simibubi/create/content/trains/GlobalRailwayManager.java

+		for (Train train : movingTrains){
+			train.updateCollisionCache();
+			train.earlyTick(level);}


Where will it update? On this line you specifically skip trains that do not have a collision cache: https://github.com/Creators-of-Create/Create/pull/9513/files#diff-671a8e994774f6be471b30fd8459587f56fd246119f83bb82b47b47e0056608cR732

I believe it doesn't crash or anything, but I'm not convinced that there aren't false negatives in the collision checks under the current iteration of this PR

I agree, do it

Jozufozu · 2025-11-22T21:53:21Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java


-		Vec3 start = (speed < 0 ? trailingPoint : leadingPoint).getPosition(graph);
-		Vec3 end = (speed < 0 ? leadingPoint : trailingPoint).getPosition(graph);
+		Vec3 start = collisionCache.start[0];


I do not see anything in this function that would indicate the order of the cache is reversed based on speed https://github.com/Creators-of-Create/Create/pull/9513/files#diff-671a8e994774f6be471b30fd8459587f56fd246119f83bb82b47b47e0056608cR646

Earlier in the train tick we potentially invalidate the cache, leaving it null

Jozufozu · 2025-11-22T21:54:14Z

src/main/java/com/simibubi/create/content/trains/entity/Train.java


-		Vec3 start = (speed < 0 ? trailingPoint : leadingPoint).getPosition(graph);
-		Vec3 end = (speed < 0 ? leadingPoint : trailingPoint).getPosition(graph);
+		if (collisionCache == null) {


Why do this here if all trains are supposed to have it baked ahead of time here?
https://github.com/Creators-of-Create/Create/pull/9513/files#diff-f2e214ec2c56dc28c4a2596c6113202b29392f9dc209be510cce0bed03be4042R222

stacode123 added 2 commits November 8, 2025 15:15

Implement a cache for train collisions for better performance on larg…

f18968c

…e networks

Remove redundant fallback

3f80853

Jozufozu self-requested a review November 8, 2025 21:19

Jozufozu requested changes Nov 8, 2025

View reviewed changes

Address comments and further improve performance by reducing updateCo…

7e58290

…llisionCache calls.

Jozufozu requested changes Nov 9, 2025

View reviewed changes

Address comments

956dcc1

stacode123 requested a review from Jozufozu November 16, 2025 12:48

Jozufozu requested changes Nov 22, 2025

View reviewed changes

Implements a cache for train collision checks for better performance #9513

Are you sure you want to change the base?

Implements a cache for train collision checks for better performance #9513

Conversation

stacode123 commented Nov 8, 2025

Uh oh!

sleepy-evelyn commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jozufozu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacode123 commented Nov 8, 2025

Uh oh!

stacode123 commented Nov 9, 2025

Uh oh!

sleepy-evelyn commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stacode123 commented Nov 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jozufozu Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sleepy-evelyn commented Nov 8, 2025 •

edited

Loading

sleepy-evelyn commented Nov 9, 2025 •

edited

Loading

Jozufozu Nov 9, 2025 •

edited

Loading